I think that we should check error codes instead of just assuming that it works here...

In order to be sure I've correctly understood you, below my attempt at rewording our previous discussion

• Some functions in libgit2 returns 0/1 values that should be understood as a boolean value.
• Current LibGit2Sharp implementation assumes that every non zero return value means true.
• Later, those libgit2 functions may be updated and also return error codes. Provided this happens, LibGit2Sharp might misunderstand an error code and consider it as a true value.
• This PR proposes to modify the managed implementation in the Proxy layer to account for potential future changes.

Provided I correctly got the underlying intent, I don't disagree. This may indeed be beneficial and add some kind of safety net. However, I wonder if we could even push this a little bit further.

Libgit2 exposes 4 kinds of functions:

1. Those which do not return anything (git_repository_set_config(), ...)
2. Those which return either 0 or an error code (git_index_conflict_remove(), ...)
3. Those which return either 0 or 1 (git_index_has_conflicts()...)
4. Those which return 0, an error code or a positive integer (git_branch_tracking_name(), ...)

How about modifying the Ensure type to explicitly cope with each of those cases by dropping Ensure.Success and replacing it with something like:

1. N/A
2. Ensure.StandardOutput
3. Ensure.BoolOutput
4. Ensure.IntegerOutput

Ensure.Bool would also take care of checking the potential negative output.

Note: Knowing that I'm awfully bad at Naming Things, I'm of course completely open to counter-proposals regarding the names of the Ensure methods.

Note 2: @phkelley recently fixed git_index_find() to have the position returned as an out parameter. If this turns into a recommended pattern, the last kind of function would disappear

Beside this, I've got some more nitpicks for you:

• I think that Travis choked on your proposal 😜
• When potentially retrieving an error message (through an Ensure.Success() call), it's recommended to wrap the native invocations with a using (ThreadAffinity())

This seems like smart future-proofing to me.

As for names, I suggest:

void Ensure.Success(res); // I don't see a reason to change this
bool Ensure.BooleanResult(res);
int Ensure.Int32Result(res);

In order to have all the methods on the same semantic level, how about

void Ensure.Success(res); -> void Ensure.ZeroResult(res);

Thank you, @nulltoken, as usual you were much more eloquent than I was. Love @dahlbyk 's suggestions.

(I realize this isn't super high-pri but I was thinking about changing a func in libgit2 and suddenly worried about the binding work reaction and wished there wasn't any!

I realize this isn't super high-pri

To me, preventing the developer from shooting in the user's foot is quite a high priority 😉

If you find the time to work on this PR, I'll be happy to merge it. However, if your Todo list is already full, please just say so, I'm sure somebody will be happy to tackle this.

As one who shoots ones self in ones foot quite frequently, I appreciate that.

I'm happy to go down this road - my todo list just got a lot shorter!

void Ensure.ZeroResult(res);

👍

OK, I think I got everything...! I noticed that the binding for git_index_find is outdated, I'll submit a similar PR for that.

💟 Long live the Interop King!